@llvm/pr-subscribers-clang

Author: None (higher-performance)

diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 2f71053d030f68..f8529105f8d034 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -5440,6 +5440,43 @@ AST_MATCHER(FunctionDecl, isDefaulted) {
   return Node.isDefaulted();
 }
 
+/// Matches trivial methods and types.
+///
+/// Given:
+/// \code
+///   class A { A(); };
+///   A::A() = default;
+///   class B { B() = default; };
+/// \endcode
+/// cxxMethodDecl(isTrivial())
+///   matches the declaration of B, but not A.
+AST_POLYMORPHIC_MATCHER(isTrivial,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES(CXXMethodDecl,
+                                                        CXXRecordDecl)) {
+  if (const auto *E = dyn_cast<CXXMethodDecl>(&Node))
+    return E->isTrivial();
+  if (const auto *E = dyn_cast<CXXRecordDecl>(&Node)) {
+    const auto *Def = Node.getDefinition();
+    return Def && Def->isTrivial();
+  }
+  return false;
+}
+
+/// Matches trivially copyable types.
+///
+/// Given:
+/// \code
+///   class A { A(const A &); };
+///   A::A(const A &) = default;
+///   class B { B(const B &) = default; };
+/// \endcode
+/// cxxMethodDecl(isTriviallyCopyable())
+///   matches the declaration of B, but not A.
+AST_MATCHER(CXXRecordDecl, isTriviallyCopyable) {
+  CXXRecordDecl *Def = Node.getDefinition();
+  return Def && Def->isTriviallyCopyable();
+}
+
 /// Matches weak function declarations.
 ///
 /// Given:
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
index 87774b00956a5a..9c648588ba970c 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1849,6 +1849,33 @@ TEST_P(ASTMatchersTest, IsDeleted) {
                       functionDecl(hasName("Func"), isDeleted())));
 }
 
+TEST_P(ASTMatchersTest, IsTrivial) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+
+  EXPECT_TRUE(notMatches("class A { A(); };",
+                         cxxRecordDecl(hasName("A"), isTrivial())));
+  EXPECT_TRUE(matches("class B { B() = default; };",
+                      cxxRecordDecl(hasName("B"), isTrivial())));
+
+  EXPECT_TRUE(notMatches("class A { ~A(); }; A::~A() = default;",
+                         cxxMethodDecl(hasName("~A"), isTrivial())));
+  EXPECT_TRUE(matches("class B { ~B() = default; };",
+                      cxxMethodDecl(hasName("~B"), isTrivial())));
+}
+
+TEST_P(ASTMatchersTest, IsTriviallyCopyable) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+
+  EXPECT_TRUE(notMatches("class A { ~A(); }; A::~A() = default;",
+                         cxxRecordDecl(hasName("A"), isTriviallyCopyable())));
+  EXPECT_TRUE(matches("class B { ~B() = default; };",
+                      cxxRecordDecl(hasName("B"), isTriviallyCopyable())));
+}
+
 TEST_P(ASTMatchersTest, IsNoThrow_DynamicExceptionSpec) {
   if (!GetParam().supportsCXXDynamicExceptionSpecification()) {
     return;

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

I remember back in the day that adding new matchers was a problem because it grew the size of the files beyond some supported compiler's limit.

Could @AaronBallman or someone else knowledgable about this(cc @kadircet) take a look and confirm adding matchers is fine now?

I remember back in the day that adding new matchers was a problem because it grew the size of the files beyond some supported compiler's limit.

Could @AaronBallman or someone else knowledgable about this(cc @kadircet) take a look and confirm adding matchers is fine now?

We generally only add matchers when there's a need for them to reside in-tree (e.g., a few clang-tidy checks will use them); otherwise we recommend that folks define their own custom matchers. The reason is mostly because compile time overhead for compiling ASTMatchers.h is... huge... due to the template instantiations, so we usually want there to be a need before adding any.

Is there a planned need here?

@AaronBallman: Oh I see. I didn't have any plans to upstream any matchers that used this, though I suppose I could. The check I had in mind though ostensibly could be in-tree, if you guys were interested. Though now that I think about it more, I think it would need isTriviallyDestructible rather than isTrivially. It basically detects whether there is an assignment of the form

<trivialy_destructible_type> = <destructible_type>;

which is frequently (but not always) dubious.
The canonical example here would be:

std::string Get();
std::string_view sv;
sv = Get();  // dangling pointer

which results in a dangling pointer. -Wdangling would catch the obvious candidates annotated with lifetimebound, but this check would surface things that might not be annotated.

Thanks for the details! I'd say let's hold off on adding these matchers; if you have a check that needs them, then you could look through clang-tidy to see if other checks have also added a local matcher for the same thing, and if there's two or more matchers needing it, then we'll add it as a global matcher.

As for the check itself, that looks like an interesting one!

@AaronBallman that makes sense, thanks! For isTrivial, I do see it is used in two places, but they're not fully compatible (one operates on QualType, and neither works on methods):

• llvm-project/clang-tools-extra/clang-tidy/bugprone/UnusedLocalNonTrivialVariableCheck.cpp

  Lines 33 to 36 in ebe77cc

  	AST_MATCHER(QualType, isTrivial) {
  	return Node.isTrivialType(Finder->getASTContext()) ||
  	Node.isTriviallyCopyableType(Finder->getASTContext());
  	}

• llvm-project/libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp

  Line 16 in 83ce139

  AST_MATCHER(clang::CXXRecordDecl, isTrivial) { return Node.isTrivial(); }

Is that worth adding/migrating to?

Otherwise, for isTriviallyCopyable, I only see one matcher with that name, but it's incompatible.

While the libc++ checks are in the same tree as clang-tidy itself we (libc++) don't actually consider them in-tree, since they are compiled as a plugin against a clang-tidy that's already installed on the system.

Thanks! I'll close this for now then.